Bugfixes - #95
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
=======================================
Coverage 98.87% 98.88%
=======================================
Files 20 20
Lines 976 984 +8
=======================================
+ Hits 965 973 +8
Misses 11 11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks. Some comments in code. I also think the logic could be better. For _get_required_backgrounds we actually need a background where the time_per_pdf is > experiment.time_per_pdf. By working out exactly what the max_time_per_pdf is upfront and saying we need exactly that we lean more heavily on the assumption that everything is queued at once (which is a valid assumption right now but we should try and not use it as much as possible). I think the logic instead should be, something like:
for task in task:
if tiled_background.time > task.time:
task.background = tiled_background
else:
backgrounds.append(get_required_backgrounds(task))
actual_background = backgrounds[0]
for potential_background in backgrounds:
if potential_background.time > actual_background.time:
actual_background = potential_background
This obviously gets more complex again when we add different capillaries/temps but I think the general pattern of :
- Go through all the tasks and assign them tiled backgrounds if appropriate, otherwise make a set of backgrounds they would need
- Go through these backgrounds and sanitise them
makes sense
| name="background_scan", id="", data={"background": background} | ||
| name="background_scan", | ||
| id="", | ||
| data={"background": background, "time_per_pdf": 10}, |
There was a problem hiding this comment.
Should: Should this not be
| data={"background": background, "time_per_pdf": 10}, | |
| data={"background": background, "time_per_pdf": background.time_per_pdf}, |
There was a problem hiding this comment.
Yes, good spot thanks
| if isinstance(task.experiment, Experiment) | ||
| and "time_per_pdf" in task.experiment.experiment_definition.data | ||
| ] | ||
| max_time_per_pdf = max(pdf_times) if pdf_times else 10 |
There was a problem hiding this comment.
Should: I think there not being pdf_times is probably an error
There was a problem hiding this comment.
For now I'll let it KeyError, once we get the types from the graph schema we can validate the whole data dict
|
Rest of the comments should be covered by #80 |
2 participants
Bugs:
_sync,self._queuewas pointing to the same object asself._last_good_contents["queue"]same forqueue_historyetc. This meant thatself._last_good_contentsfrom then on changed whenever self._queue changed, defeating its whole purpose. Fixed by doing adeepcopytime_per_pdffield, but the background scans inserted by the plugin didn't populate this field. Have added this in based on the maxtime_per_pdfin the queue, and a test that would have caught thisAlso adds some more logging that was helpful when debugging the above